-
Notifications
You must be signed in to change notification settings - Fork 2.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Progress bar in the status message modal + auto-refresh of the modal #2108
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is fantastic. Also the information can (theoretically) be used by so much else (like scheduling ordering).
Looks good to me. @daveFNbuck also mentioned implementing something like this once. :)
class TaskProgressPercentageTest(LuigiTestCase): | ||
|
||
def test_run(self): | ||
percentage = 30 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think avoiding a variable makes it more readable. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This all looks cool. Have you used this in production in a while now? Does it work smooth? Can you just go over my comments and then let me know if this is ready to be merged or not.
import luigi.scheduler | ||
import luigi.worker | ||
|
||
luigi.notifications.DEBUG = True |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove this line
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @Tarrasch, yes we're using it in production for a while and it's pretty smooth.
I try to take some time over the week-end to do the changes and I let you know.
task = self._state.get_task(task_id) | ||
return {"taskId": task_id, "progressPercentage": task.progress_percentage} | ||
else: | ||
return {"taskId": task_id, "progressPercentage": None} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wait a second. Why don't we return None
/{}
here or some sort of failure indication, since the task id didn't exist right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes right, I tried to stick to what was already in place in get_task_status_message
or fetch_error
.
I guess the case may happen if a RPC request is still coming on a given taskId
while a task is dead (finished a long time ago but browser still opened in the progress bar display).
In this case luigi/static/visualiser/js/visualiserApp.js
should handle it gracefully (hiding the no longer relevant HTML objects).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @dlstadther, just checked again and for me everything looks ok?
task = self._state.get_task(task_id) | ||
return {"taskId": task_id, "progressPercentage": task.progress_percentage} | ||
else: | ||
return {"taskId": task_id, "progressPercentage": None} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes right, I tried to stick to what was already in place in get_task_status_message
or fetch_error
.
I guess the case may happen if a RPC request is still coming on a given taskId
while a task is dead (finished a long time ago but browser still opened in the progress bar display).
In this case luigi/static/visualiser/js/visualiserApp.js
should handle it gracefully (hiding the no longer relevant HTML objects).
Thanks @fvinas, I reworded the final commit a bit. I wasn't sure what "modal" means here so I removed it before merging. :) Anyway expect a release with this patch soon! |
Description
Added the ability to display a progress bar in the status message modal using
task.set_progress_percentage()
+ automatically refreshing the modal (for both status message and the progress bar) to follow progress dynamically while it's open.Motivation and Context
More visual & practical way to track the progress of running tasks inside Luigi (i.e. not using an external tracking URL). Status message can now be dedicated to text messages instead of progress tracking.
Brings dynamic view on a running task when the status message modal is open (elsewhere the UI remains static for now)
Have you tested this? If so, how?
Included unit tests, seems to work well in my environment.